chore: [WIP] experimental parallel kokoro system test#17608
chore: [WIP] experimental parallel kokoro system test#17608chalmerlowe wants to merge 9 commits into
Conversation
chalmerlowe
changed the title
chalmerlowe
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces parallel execution for system tests in .kokoro/system.sh with concurrency limits and log segregation, and isolates gcloud configurations per run using unique temporary directories. The feedback suggests robust cleanup of the temporary directories using shell traps to prevent leaks if commands fail under set -e, sanitizing package names used in directory creation, and refactoring the duplicate process reaping logic into a helper function to improve maintainability.
chalmerlowe
added
the
do not merge
|
Applied Do Not Merge, simply because we will want to remove the throw-away comments just prior to merging. |
chalmerlowe
added
kokoro:force-run
chalmerlowe
changed the title
chalmerlowe
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
chalmerlowe
changed the title
|
Something in this PR is off. Hold off on reviewing it. |
|
|
||
| printf '%s\n' "${PACKAGES_TO_TEST[@]}" | xargs -P "$MAX_JOBS" -I {} bash -c 'run_package_test "{}" > "$LOG_DIR/{}.log" 2>&1 || touch "$LOG_DIR/{}.failed"' | ||
|
|
||
| reap_parallel_results || RETVAL=1 |
There was a problem hiding this comment.
It looks like right now, the main cloud-devrel/client-libraries/python/googleapis/google-cloud-python/presubmit/system target always returns 0. Is that intended? The individual sub-targets are still reporting properly, but it would probably be better to have the main target fail if any others do
Maybe the exit ${RETVAL} line was removed by mistake?
There was a problem hiding this comment.
will ensure that last line gets in there.
| local CLOUDSDK_CONFIG="${gcloud_config_dir}" | ||
|
|
||
| # 🪤 TRAP: Ensure cleanup of THIS specific temp dir on exit of this subshell | ||
| trap 'rm -rf "$gcloud_config_dir"' EXIT |
There was a problem hiding this comment.
Out of curiosity, is this required? I assumed the environment would just be destroyed after completion
| export -f run_package_test | ||
| export system_test_script PROJECT_ROOT KOKORO_GFILE_DIR | ||
|
|
||
| printf '%s\n' "${PACKAGES_TO_TEST[@]}" | xargs -P "$MAX_JOBS" -I {} bash -c 'run_package_test "{}" > "$LOG_DIR/{}.log" 2>&1 || touch "$LOG_DIR/{}.failed"' |
There was a problem hiding this comment.
nit: This is a pretty dense line. A comment explaining that this triggers run_package_test for each package, and writes to LOG_DIR would be helpful
| MAX_JOBS=${MAX_JOBS:-4} | ||
|
|
||
| # Temporary directory for clean log segregation | ||
| LOG_DIR=$(mktemp -d -t test-logs-XXXXXX) |
There was a problem hiding this comment.
IIRC, the expectation is to write individual package logs to ${KOKORO_ARTIFACTS_DIR}/${pkg}/sponge_log.log".
In theory, this will persist the logs as an artifact, which would let us view logs for each individual target under the "Target Log" tab.
This doesn't work properly in the default BTX view. But if you click through to Sponge, it renders nicely there (invocation details > Sponge URL)
I don't think this really matters, but consider saving individual logs to that artifact directory any way, just in case BTX starts working for us some day
3 participants
Warning
Something in this PR is off. Hold off on reviewing it.
Note
Results
Baseline: 2 hours 5 mins
With parallelization: 42 mins